fix: Keep a failing background watcher from breaking a successful actor call - #1027
Draft
vdusek wants to merge 9 commits into
Draft
fix: Keep a failing background watcher from breaking a successful actor call#1027vdusek wants to merge 9 commits into
vdusek wants to merge 9 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1027 +/- ##
=======================================
Coverage 94.95% 94.96%
=======================================
Files 58 58
Lines 5436 5463 +27
=======================================
+ Hits 5162 5188 +26
- Misses 274 275 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…of a request count
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix
actor.call()with the defaultlogger='default'could raise on a run that had already succeeded. A status poll that failed after retries left the exception on the watcher task;stop()then cancelled an already-finished task andawaited it, re-raising pastexcept asyncio.CancelledError. The sync twin leaked the same failure out of its thread.Fixes, all on that path:
StreamedLog._stream_logalready had — warn onis_timeout_error,logger.exceptionotherwise — so a failed poll is reported instead of surfacing as a failure of the run.StreamedLog.stop()closes the stream response and bounds its join with_stop_timeout_s(5s), instead of joining unbounded on a stream that may not speak for hours.Closing the response alone is not enough: on impit 0.13.2
Response.close()returns immediately and setsis_closed=True, but a reader blocked initer_bytes()stays blocked, and closing theClientdoes not release it either. The close is kept because it is correct for a custom HTTP client; the bounded join is what makesstop()return.5 new unit tests plus an integration test that fails a real run's background status polls, each red before the fix.
Why
stop()is bounded this wayThe bound is the third pass over the same problem, so for context:
stop()by requesting the log stream with a 30s read timeout.timeoutto the whole request, streamed body included, so a run that logged for longer than the bound was cut off mid-stream withimpit.TimeoutException— the user report impit.TimeoutException in _streamed_log.py after successful actor completion #945. The stream has been requested withno_timeoutsince, and fix: prevent Actor log-streaming thread from crashing on stream timeout #944 recorded the resulting unboundedstop()as a known limitation, on the grounds thatactor.call()is unaffected because run finish sends EOF.stop()without touching the stream request: a 5s cap on the join, plus a close on the response._stream_timeoutstaysno_timeout— still asserted bytest_streamed_log_sync_requests_stream_with_no_timeout— so impit.TimeoutException in _streamed_log.py after successful actor completion #945 cannot regress. What is fixed is the case fix: prevent Actor log-streaming thread from crashing on stream timeout #944 left open: a manualstop()on a stream that has gone quiet.Trade-off: a thread that outlives the 5s cap keeps running as a daemon, so
stop()can return before the tail is flushed. It does not happen on theactor.call()path, where the run's EOF ends the thread first — measured at 0.000s over repeatedapify/hello-worldruns.✍️ Drafted by Claude Code